Introduce the `DynamicFileCatalog` in `datafusion-catalog` #11035

goldmedal · 2024-06-20T18:05:15Z

Which issue does this PR close?

Closes #10986 .

Rationale for this change

Follow the idea of #4838 to implement DynamicFileCatalog in datafusion-catalog. Users can enable this feature for an existing SessionContext by SessoinContext::enable_url_table.

let ctx = SessionContext::new().enable_url_table();
ctx.sql("SELECT * FROM 'tests/data/example.csv' as example").await?.show().await?;

What changes are included in this PR?

Implement SessionStore to access SessionState for he runtime table building.
In datafusion-cli, DynamicFileCatalog is renamed to DynamicObjectStoreCatalog because it's responsible for registering the required object store now.

Are these changes tested?

yes

Are there any user-facing changes?

add new API of SessoinContext: enable_url_table
add new feature: home_dir for the URL table.

goldmedal · 2024-06-20T18:07:32Z

datafusion/core/src/execution/context/mod.rs

@@ -305,10 +306,18 @@ impl SessionContext {

    /// Creates a new `SessionContext` using the provided [`SessionState`]
    pub fn new_with_state(state: SessionState) -> Self {
+        let state_ref = Arc::new(RwLock::new(state.clone()));
+        state
+            .schema_for_ref("datafusion.public.xx")


It just a workaround to get DynamicFileSchemaProvider. I'll refactor it later.

goldmedal · 2024-06-20T18:09:16Z

datafusion/core/src/catalog/dynamic_file_schema.rs

+    async fn table(&self, name: &str) -> Result<Option<Arc<dyn TableProvider>>> {
+        let inner_table = self.inner.table(name).await?;
+        if inner_table.is_some() {
+            return Ok(inner_table);
+        }
+        let optimized_url = substitute_tilde(name.to_owned());
+        let table_url = ListingTableUrl::parse(optimized_url.as_str())?;
+        let state = &self
+            .state_store
+            .get_state()


SchemaProvider::table won't have SessionState by default. We should get it from StateStore.

goldmedal · 2024-06-20T18:10:23Z

datafusion-examples/examples/dynamic_csv_sql.rs

+    let df = ctx
+        .sql(
+            format!(
+                r#"SELECT column_1, MIN(column_12), MAX(column_12)


The column is c1 actually. There're some issues about getting CSV header automatically.

ok, I found that this is the default behavior for ListTable. The dynamic query in datafusion-cli is the same as this. I think we don't need to change it in this PR.

This behavior is controlled by the config option in the current session context. We can create the session context like

let cfg = SessionConfig::new().set_str("datafusion.catalog.has_header", "true"); let ctx = SessionContext::new_with_config(cfg);

to enable the header scanning.

goldmedal · 2024-06-21T18:53:30Z

datafusion/sqllogictest/test_files/array_query.slt

@@ -19,7 +19,7 @@

 # Make a table with multiple input partitions
 statement ok
-CREATE TABLE data AS


Seems there're a file or directory called data in the working directory. However, I think it's really weird. IMO, we should only match a string literal like select * from '/xxx/aaa/data.csv' but the normal identifier of a table also be matched.
I'll do more research for it.

ok. I did an experimental for DataFusion v39.0.0 like

(base) jax@jax-canner:~/git/datafusion/testing/data/csv (master~4) $ mkdir datas (base) jax@jax-canner:~/git/datafusion/testing/data/csv (master~4) $ cp aggregate_test_100.csv datas (base) jax@jax-canner:~/git/datafusion/testing/data/csv (master~4) $ datafusion-cli DataFusion CLI v39.0.0 > select * from datas; +----------+----------+----------+----------+-------------+----------------------+----------+----------+------------+----------------------+-------------+---------------------+--------------------------------+ | column_1 | column_2 | column_3 | column_4 | column_5 | column_6 | column_7 | column_8 | column_9 | column_10 | column_11 | column_12 | column_13 | +----------+----------+----------+----------+-------------+----------------------+----------+----------+------------+----------------------+-------------+---------------------+--------------------------------+

I created a folder datas and place a csv in this, then start the cli in this path. I can directly query this folder without single-quote string literal.

For a single file, it doesn't work

(base) jax@jax-canner:~/git/datafusion/testing/data/csv (master~4) $ cp aggregate_test_100.csv data (base) jax@jax-canner:~/git/datafusion/testing/data/csv (master~4) $ datafusion-cli DataFusion CLI v39.0.0 > select * from data; Error during planning: table 'datafusion.public.data' not found

I think it should be an issue to match only the string literal as the dynamic file table name.

goldmedal · 2024-06-22T04:29:30Z

datafusion/sqllogictest/test_files/arrow_files.slt

@@ -64,6 +72,13 @@ SELECT * FROM arrow_partitioned ORDER BY f0;
 3 baz true 456
 4 NULL NULL 456

+# dynamic select arrow file in the folder
+query ITB
+SELECT * FROM '../core/tests/data/partitioned_table_arrow/part=123' ORDER BY f0;


Dynamic query doesn't support the partitioned folder. It could be an enhancement issue for it.

alamb · 2024-06-22T11:46:12Z

Thanks @goldmedal -- I hope to review this PR this wekeend, likely tomorrow

alamb · 2024-06-28T00:24:48Z

I am sorry for the delay -- I plan to review this tomorrow

alamb

First of all, thank you so much @goldmedal -- this is really cool

I have a few thoughts

Security implications

I worry about its security implications (this will now allow anything built on DataFusion 's SessionContext to read arbitrary local files even if https://docs.rs/datafusion/latest/datafusion/execution/context/struct.SQLOptions.html#method.with_allow_dml is disabled

I think we should therefore not register this provider with the SessionContext by default and instead add some examples showing how to register it by itself

Structure / dependencies

As the code is currently structured (not related to this PR) it seems like this has to go into the core crate. It seems to me it would be better if we could find some way to start breaking up the core (e.g. remove the catalog providers, etc)

I am still thinking about this

datafusion-examples/examples/csv_sql.rs

datafusion/sqllogictest/test_files/describe.slt

datafusion/core/src/catalog/dynamic_file_schema.rs

datafusion/core/Cargo.toml

alamb · 2024-08-26T19:29:35Z

Thanks @goldmedal -- I'll try and review this shortly

goldmedal · 2024-09-04T15:57:50Z

datafusion-cli/Cargo.lock

@@ -584,9 +584,9 @@ dependencies = [

 [[package]]
 name = "aws-sdk-sts"
-version = "1.41.0"


If we execute cargo update, the version will be updated to 1.41.0 which isn't compatible with Rust 1.76, then the msrv check will fail. I'm not sure if there is a better way to limit the version in the cargo toml file to avoid the update command updates an invalid version 🤔

Yes I have actually worked around this on #12032 (comment)

alamb · 2024-09-05T11:09:31Z

I will review this PR over the next day or two

alamb

TLDR is I think this PR is really nice @goldmedal -- the code is well structured and commented, and I think this will be a very useful feature for users of DataFusion

What I would like to do is to try and create an example in datafusion-examples of a program that run queries from arbitrary URLs. SELECT * from 'my-s3://foo.com/data'

I'll try and work on that today or tomorrow -- I suspect in trying to make that example it will become apparent what, if any, gaps remain in the APIs in terms of registering ObjectStorees etc.

goldmedal · 2024-09-07T11:59:51Z

datafusion-examples/examples/external_dependency/query-aws-s3.rs

+    // dynamic query by the file path
+    ctx.enable_url_table();
+    let df = ctx
+        .sql(format!(r#"SELECT * FROM '{}' LIMIT 10"#, &path).as_str())
+        .await?;
+
+    // print the results
+    df.show().await?;


Thanks, @alamb. I added a simple s3 example here. I hope it is what you want or that it could inspire you for a new example.

No actually this is perfect -- thank you @goldmedal

alamb

Thanks again @goldmedal

I plan to leave this PR open until Monday so anyone else who is interested can take a look at it prior to merge.

I left some suggestions on how to potentially improve the comments / documentation, but I think we (I can do it) as a follow on PR too

alamb · 2024-09-07T12:06:19Z

datafusion/core/src/datasource/dynamic_file.rs

+#[derive(Default)]
+pub struct DynamicListTableFactory {
+    /// The session store that contains the current session.
+    session_store: Arc<SessionStore>,


FWIW since the SessionStore has an Arc inside it, this extra level of Arc may not be necessary

alamb · 2024-09-07T12:08:00Z

datafusion/core/src/execution/context/mod.rs

+    /// # use datafusion::{error::Result, assert_batches_eq};
+    /// # #[tokio::main]
+    /// # async fn main() -> Result<()> {
+    /// let cfg = SessionConfig::new().set_str("datafusion.catalog.has_header", "true");


I think @korowa changed the default so that has_header is true by default now so this line is unecessary

alamb · 2024-09-07T12:09:41Z

datafusion/core/src/execution/context/mod.rs

+    /// Enable the dynamic file query for the current session.
+    /// See [DynamicFileCatalog] for more details
+    ///


Suggested change

/// Enable the dynamic file query for the current session.

/// See [DynamicFileCatalog] for more details

///

/// Enable dynamic file querying for the current session.

///

/// This allows queries to directly access arbitrary file names via SQL like

/// `SELECT * from 'my_file.parquet'`

/// so it should only be enabled for systems that such access is not a security risk

///

/// See [DynamicFileCatalog] for more details

///

alamb · 2024-09-07T12:10:09Z

datafusion/core/src/execution/context/mod.rs

+        let path = PathBuf::from(env!("CARGO_MANIFEST_DIR"));
+        let path = path.join("tests/tpch-csv/customer.csv");
+        let url = format!("file://{}", path.display());
+        let cfg = SessionConfig::new().set_str("datafusion.catalog.has_header", "true");


likewise here it isn't necessary to set the has_header flag anymore I don't think

alamb · 2024-09-07T12:10:49Z

datafusion/sqllogictest/test_files/arrow_files.slt

@@ -43,6 +43,10 @@ SELECT * FROM arrow_simple
 3 baz false
 4 NULL true

+# url table is only supported by DynamicFileCatalog


I think adding context here about why this is an important test would make it easier to understand for future readers

Suggested change

# url table is only supported by DynamicFileCatalog

# Ensure that local files can not be read by default (a potential security issue)

# (url table is only supported when DynamicFileCatalog is enabled)

alamb · 2024-09-07T12:13:24Z

datafusion/sqllogictest/test_files/dynamic_file.slt

+~id9~ ~value9~
+
+query TTT
+DESCRIBE '../core/tests/data/aggregate_simple.csv';


alamb · 2024-09-07T12:17:03Z

datafusion/catalog/src/dynamic_file/catalog.rs

+//! dynamic_file contains [`DynamicFileCatalog`] that creates tables from file paths
+//! if the wrapped [`CatalogProviderList`] doesn't have the table provider.


I suggest making module level documentation (which is rendered typically as a single line) point to the main struct/trati

Suggested change

//! dynamic_file contains [`DynamicFileCatalog`] that creates tables from file paths

//! if the wrapped [`CatalogProviderList`] doesn't have the table provider.

//! [`DynamicFileCatalog`] that creates tables from file paths

alamb · 2024-09-07T12:20:56Z

datafusion/catalog/src/dynamic_file/catalog.rs

+/// Implements the [DynamicFileSchemaProvider] that can create tables provider from the file path.
+///
+/// The provider will try to create a table provider from the file path if the table provider
+/// isn't exist in the inner schema provider. The required object store must be registered in the session context.


I don't understand the reference to object_store here -- it is up to the UrlTableFactory to handle resolving urls to TableProviders, right?

Yes, for example, how DynamicListTableFactory resolves the URL depends on what the session context registers as the object store.
Indeed, it's a legacy comment (before moving to datafusion-catalog). We don't need to mention the object store here. 🤔

I will remove the part of the object store here. Thanks for mentioning it.

alamb · 2024-09-07T12:22:49Z

datafusion/sqllogictest/test_files/dynamic_file.slt

+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+


Suggested change

#

# Note: This file runs with a SessionContext that has the `enable_url_table` flag set

#

alamb · 2024-09-07T12:24:17Z

datafusion/core/src/execution/context/mod.rs

+        let session_state = SessionStateBuilder::new()
+            .with_default_features()
+            .with_config(cfg)
+            .build();
+        let ctx = SessionContext::new_with_state(session_state).enable_url_table();
+        let result = plan_and_collect(
+            &ctx,
+            format!("select c_name from '{}' limit 3;", &url).as_str(),
+        )
+        .await?;


I wonder if it would be possible here to add enable_url_table as a argument to SessionStateBuilder

So it could look like

let session_state = SessionStateBuilder::new() .with_default_features() .with_config(cfg) .enable_url_table() .build(); let ctx = SessionContext::from(session_state);

🤔

(we can do this as a follow on as well)

Sure. I think it's a good idea. We can have a flag in the builder. If it's true, we can wrap the catalog list implicitly. It may need some additional implementation and test cases. I prefer to do it in the follow-up PR.

goldmedal · 2024-09-07T15:56:10Z

I plan to leave this PR open until Monday so anyone else who is interested can take a look at it prior to merge.

👍

I left some suggestions on how to potentially improve the comments / documentation, but I think we (I can do it) as a follow on PR too

Thanks @alamb I try to address the suggestions here. Only about the comment related to SessionStateBuilder, I prefer to do it in another PR.

goldmedal added 2 commits June 21, 2024 01:44

early draft

37b5526

fmt

ad1a854

github-actions bot added the core Core DataFusion crate label Jun 20, 2024

goldmedal commented Jun 20, 2024

View reviewed changes

goldmedal mentioned this pull request Jun 20, 2024

Implement DynamicTableProvider in DataFusion Core #10986

Closed

goldmedal added 8 commits June 21, 2024 23:44

add example for dynamic file query

97ea11c

add test and refactor

2729c49

clippy and add doc

6f86577

cargo fmt

c91cdc6

extract substitute_tilde function

3306df6

fix the error handling

d82b273

fmt and clippy

c0491d5

fix test

a60eeea

github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Jun 21, 2024

goldmedal commented Jun 21, 2024

View reviewed changes

goldmedal added 3 commits June 22, 2024 11:50

fix sqllogictests

9fa01aa

ignore dirs for windows test

2ab3639

enhance the test for every file format

a8ee733

goldmedal commented Jun 22, 2024

View reviewed changes

disable the test for windows

7faab9f

goldmedal changed the title ~~[DRAFT] Implement DynamicFileSchemaProvider in the core~~ Implement DynamicFileSchemaProvider in the core Jun 22, 2024

goldmedal marked this pull request as ready for review June 22, 2024 05:35

This was referenced Jun 22, 2024

DataFusion weekly project plan (Andrew Lamb) - June 17, 2024 #10955

Closed

DataFusion weekly project plan (Andrew Lamb) - June 24, 2024 #11106

Closed

alamb mentioned this pull request Jun 28, 2024

Minor: fix SQLOptions::with_allow_ddl comments #11166

Merged

alamb reviewed Jun 28, 2024

View reviewed changes

goldmedal added 5 commits August 20, 2024 21:59

Merge branch 'main' into feature/10986-dynamic-table-provider

76d7fee

separate dynamic file test to another slt

f0f070b

add test for querying url table but disabled this feature

6b77b6b

add dynamic_file.slt

4e51a77

remove home_dir feature

fafc9dc

github-actions bot removed the documentation Improvements or additions to documentation label Aug 20, 2024

Merge branch 'main' into feature/10986-dynamic-table-provider

a3a4f4d

goldmedal requested a review from alamb August 26, 2024 14:14

goldmedal added 2 commits September 4, 2024 22:18

Merge branch 'main' into feature/10986-dynamic-table-provider

7dc238f

update cli lock

f7b4b8c

goldmedal force-pushed the feature/10986-dynamic-table-provider branch from 7fef748 to f7b4b8c Compare September 4, 2024 14:49

goldmedal added 3 commits September 4, 2024 23:20

fix msrv check

25d0ff6

fix msrv check

a78bd3c

rollback the lock change

edeff33

goldmedal commented Sep 4, 2024

View reviewed changes

alamb mentioned this pull request Sep 5, 2024

DataFusion weekly project plan (Andrew Lamb) - Sep 2, 2024 #12336

Closed

4 tasks

alamb approved these changes Sep 7, 2024

View reviewed changes

goldmedal commented Sep 7, 2024

View reviewed changes

alamb approved these changes Sep 7, 2024

View reviewed changes

goldmedal added 2 commits September 7, 2024 23:22

address review comment and enhance the doc

b1a922c

remove the legacy comment

e5ab14d

add missing doc

87d7503

alamb merged commit 9bc39a0 into apache:main Sep 9, 2024
26 checks passed

alamb mentioned this pull request Sep 9, 2024

Add enable_url_table as a argument to SessionStateBuilder #12394

Open

goldmedal deleted the feature/10986-dynamic-table-provider branch September 9, 2024 14:53

alamb mentioned this pull request Sep 9, 2024

Implement select * from 'foo.parquet' support datafusion-contrib/datafusion-dft#125

Open

goldmedal mentioned this pull request Sep 29, 2024

Allow DynamicFileCatalog support to query partitioned file #12671

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce the `DynamicFileCatalog` in `datafusion-catalog` #11035

Introduce the `DynamicFileCatalog` in `datafusion-catalog` #11035

goldmedal commented Jun 20, 2024 •

edited

Loading

goldmedal Jun 20, 2024

goldmedal Jun 20, 2024

goldmedal Jun 20, 2024

goldmedal Jun 21, 2024

goldmedal Jun 22, 2024

goldmedal Jun 21, 2024

goldmedal Jun 21, 2024

goldmedal Jun 22, 2024

alamb commented Jun 22, 2024

alamb commented Jun 28, 2024

alamb left a comment

alamb commented Aug 26, 2024

goldmedal Sep 4, 2024

alamb Sep 5, 2024

alamb commented Sep 5, 2024

alamb left a comment

goldmedal Sep 7, 2024

alamb Sep 7, 2024

alamb left a comment

alamb Sep 7, 2024

alamb Sep 7, 2024

alamb Sep 7, 2024

alamb Sep 7, 2024

alamb Sep 7, 2024

alamb Sep 7, 2024

alamb Sep 7, 2024

alamb Sep 7, 2024

goldmedal Sep 7, 2024

goldmedal Sep 7, 2024

alamb Sep 7, 2024

alamb Sep 7, 2024

goldmedal Sep 7, 2024

goldmedal commented Sep 7, 2024

-    /// Enable the dynamic file query for the current session.
-    /// See [DynamicFileCatalog] for more details
-    ///
+    /// Enable dynamic file querying for the current session.
+    ///
+    /// This allows queries to directly access arbitrary file names via SQL like
+    /// `SELECT * from 'my_file.parquet'`
+    /// so it should only be enabled for systems that such access is not a security risk
+    ///
+    /// See [DynamicFileCatalog] for more details
+    ///

	# url table is only supported by DynamicFileCatalog
	# Ensure that local files can not be read by default (a potential security issue)
	# (url table is only supported when DynamicFileCatalog is enabled)

		//! dynamic_file contains [`DynamicFileCatalog`] that creates tables from file paths
		//! if the wrapped [`CatalogProviderList`] doesn't have the table provider.

	//! dynamic_file contains [`DynamicFileCatalog`] that creates tables from file paths
	//! if the wrapped [`CatalogProviderList`] doesn't have the table provider.
	//! [`DynamicFileCatalog`] that creates tables from file paths

+#
+# Note: This file runs with a SessionContext that has the `enable_url_table` flag set
+#

Introduce the DynamicFileCatalog in datafusion-catalog #11035

Introduce the DynamicFileCatalog in datafusion-catalog #11035

Conversation

goldmedal commented Jun 20, 2024 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Jun 22, 2024

alamb commented Jun 28, 2024

alamb left a comment

Choose a reason for hiding this comment

Security implications

Structure / dependencies

alamb commented Aug 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Sep 5, 2024

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goldmedal commented Sep 7, 2024

Introduce the `DynamicFileCatalog` in `datafusion-catalog` #11035

Introduce the `DynamicFileCatalog` in `datafusion-catalog` #11035

goldmedal commented Jun 20, 2024 •

edited

Loading